feat(via_verifier): separate the verifier task from the withdrawal task#301
feat(via_verifier): separate the verifier task from the withdrawal task#3010xatomFusion wants to merge 3 commits intomainfrom
Conversation
… batch finality without processing withdrawals
0xatomFusion
force-pushed
the
feat/via-split-verifier-and-verfier-withdrawal
branch
from
3332eac to
32e58ba
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively separates the ZK verification task from the withdrawal processing task by introducing a new VerifierAndProcessor role. This improves modularity and allows for more flexible deployment configurations. The changes are well-structured, with updates to configuration, roles, and the node builder logic.
I've identified a few issues in the configuration handling that could lead to panics or unexpected behavior at runtime. Specifically, making some configuration fields optional requires careful handling of the None case in accessor functions. My review includes suggestions to make this more robust by returning Results or using expect() for mandatory fields for certain roles. I also found a minor inconsistency in the Makefile for the new verifier instance.
Overall, this is a good change that enhances the verifier's architecture. Addressing the configuration issues will make it production-ready.
| pub fn wallet_address(&self) -> anyhow::Result<Address> { | ||
| Ok(Address::from_str(&self.wallet_address)?.assume_checked()) | ||
| Ok(Address::from_str(&self.wallet_address.clone().unwrap())?.assume_checked()) | ||
| } |
There was a problem hiding this comment.
Calling unwrap() on self.wallet_address will cause a panic if the value is None. Since wallet_address is now an Option, this case must be handled. Given that the function already returns a Result, it's more idiomatic to propagate an error if the address is not set.
pub fn wallet_address(&self) -> anyhow::Result<Address> {
let address = self.wallet_address.as_ref().context("wallet_address is not set in ViaVerifierConfig")?;
Ok(Address::from_str(address)?.assume_checked())
}| pub fn coordinator_port(&self) -> u16 { | ||
| self.coordinator_port.unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
Using unwrap_or_default() here will cause coordinator_port() to return 0 if the port is not configured. When bind_addr() uses this, it will bind to a random ephemeral port. The Coordinator role, which is the one using this, should always have a specific port configured. Using expect() will cause a panic with a clear error message on startup if the configuration is missing, which is safer than silently using a random port.
pub fn coordinator_port(&self) -> u16 {
self.coordinator_port.expect("coordinator_port is not set in ViaVerifierConfig")
}| pub fn coordinator_http_url(&self) -> String { | ||
| self.coordinator_http_url.clone().unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
Using unwrap_or_default() will return an empty string if coordinator_http_url is None. This will lead to invalid URLs (e.g., "/session") being constructed, causing runtime errors in network requests. For roles that require this URL, it should be mandatory. Using expect() will ensure that the configuration is present, causing an early panic if it's missing.
pub fn coordinator_http_url(&self) -> String {
self.coordinator_http_url.clone().expect("coordinator_http_url is not set in ViaVerifierConfig")
}| # Run the basic setup workflow in verifier | ||
| .PHONY: via-restart-verifier-1 | ||
| via-restart-verifier-1: base verifier |
There was a problem hiding this comment.
The comment for via-restart-verifier-1 seems to be a copy-paste from via-verifier-1 and doesn't reflect that it's a restart command. Additionally, a restart command typically shouldn't run init (which is part of the base target). For consistency with via-restart-verifier, it would be better to use env-soft which is more appropriate for a restart operation.
# Restart the verifier-1
.PHONY: via-restart-verifier-1
via-restart-verifier-1: env-soft verifier
3 participants
What ❔
VerifierAndProcessorrole that handles both ZK verification and withdrawal processing.verifier-1Why ❔
This PR introduces a dedicated Verifier role that performs only ZK verification and participates in batch finalization. Additionally, a VerifierAndProcessor role is able to perform both verification and withdrawal processing.
Testing
via-verifier-1make via-verifier, then wait for it to sync and the withdrawal to be processed.Checklist
zkstack dev fmtandzkstack dev lint.